Needs comment

I'd probably just call this CloudData or something simple like that, since it reads and writes. If you want to keep the current name, probably should be Querier.

+1. CloudQueryer isn't a great name; I couldn't think of a better one. I don't like CloudData either because it's not the actual data, it's the thing that can read/write the data.

CloudReadWriter
/me ducks

CloudMetadataStore? Similar to how we have jujuclient.ControllerStore, which is composed of jujuclient.Controller{Updater,Remover,Getter}.

Renamed to CloudMetadataStore.

Why mock out the parsing functions? They shouldn't need to change for tests, right? At best, if you want to avoid writing to disk during tests, make it take an io.Reader or something.

Remember I used the phrase, "jedi mind trick", in one of our conversations about IoC? Here is a great example!

What we're really writing here is a command, not the parsing logic or anything to do with clouds.yaml. It's purely about interfacing with the user and then deferring logic to other components. Taking in an io.Reader is too much detail for a command; it should deal in concepts, e.g. "go parse this path" or "what do you know about this cloud?" If you defer that kind of logic to another module, you can implement/test that logic in one spot and use it all over the place. It is a very subtle (at least I think so) but important difference.

This is also why in the tests were placing files onto the file system and then reading them back out and checking against strings. Because we are not decoupled from handling cloud files, because we're not just a dumb command, this is the only way we can test.

And although testing is the first thing everyone brings up -- and it is important and what we're discussing -- it's not the main reason to do it this way. The main reason is so that the how is decoupled from the what. And you can change the how without having to also change this code. Rick and I just ran into this situation yesterday.

But we actually do need to test that the interactive add cloud code actually produces valid yaml that can be parsed. If we mock that out, then we don't know. I could go change what the parsing code expected, and these tests would all pass, but the code would be wrong and would not work in production.

From a product standpoint, yes, we need to know that it is doing the correct thing. This is the responsibility of functional and/or CI tests. The responsibility of these tests is only to know that it's gluing the invocation of commands together with the CloudMetadataStore code correctly.

We can be reasonably sure that it's doing the correct thing because of the emergent correctness between these tests and these tests.

This is why we pass in a well-tested, orthogonal module: so that this code, and these tests, don't have to re-test the same functionality. Functional tests will eclipse both of these packages, and this is why you have fewer functional tests (see this).

It doesn't have to be a functional test somewhere else. It can be a unit test right here. One of the main points of testing this code is to test that it produces valid yaml to pass to Parse. Just merely testing that it calls Parse is not good enough. Putting it into a functional test somewhere else makes failures harder to debug and more likely to be caught much later in the process, which wastes developers time. It also means that someone could easily change one of those tests in a far away place, and we'd never know we lost code coverage until something breaks... hopefully in CI, but possibly in production.

None of the tests in gh/j/j/cloud/cloud_test.go test that add cloud interactive produces valid yaml for cloud.Parse*, AFAICT.

Given that it's trivial to test here, there seems to be no reason not to.

If we had the fake call functions from within the cloud/ package, the boundaries of our tests are then different from that of our code; i.e. we'd be testing something that may not actually be happening. Not only is checking that it's calling Parse good enough, it's the only logically correct thing to do here. Likewise, I would not expect the cloud/ tests to test an interactive add-cloud run; only that when passed data x into it's functions, it returns data y. The correctness of the add-cloud command when utilized with the cloud/ package is both an emergent property of both test suites, and an explicit property of a functional test.

Functional tests are about testing that all layers are put together properly and actually work. This is the appropriate place to test that an interactive add-cloud works as intended.

Aside from the logical correctness, there are practical implications as to why you don't want to do functional/full-stack tests here. They are arguably no more difficult to write, but they are significantly more difficult to maintain. Again, please see that link about the testing pyramid. There are fewer functional/CI tests for a reason. Our current tests offer further evidence of this.

rather than passing the cloudqueryer as an argument, I would just make this and addCloud methods on addCloudCommand so they can access its member variables.

I'd much rather call real parsing functions, unless there's some reason they need to be mocked out, like if they're calling out to external services or something.

I'm not a fan of raising dependency injection to this level in the code. If we did that for every command in this list, this file would be 5000 lines long and have way too much knowledge of the entire system. I think we can encapsulate the dependency injection in NewAddCloudCommand and do the testing swap-out at that level, like have NewAddCloudCommand() call newAddCloudCommand(cloudToCommandAdapter{})

Usually what happens is that you begin finding commonality across components, and you can bundle like-concepts together into components and pass them into more than one component. This is one of the main goals/benefits! One example I can immediately think of is there is commonality between the deploy and upgrade commands. The deploy command has been factored to support injection, but upgrade has not. As soon as it is, we can factor common code (e.g. resolving charm directives to paths or previously deployed charms) into modules, test those modules once, and then pass them into the commands that need them. You want the root of your call-stack to know more about the system because this allow loser coupling. If you invert that, you get what we have now: tightly coupled components which cannot be modified/tested independently.

If the file-size is an issue, we can always split it out into more files.

I'm really not a big fan of "fluent" APIs. They're not used very much in Go, and I think for good reason. They contain more magic than I really like. For example, when I first read this, I didn't realize it was actually setting data.. it looks like it's making a call or something. It would be more clear as just adding a value to a map. IMO.

I think this would be a lot easier to read as a multiline string of some sort. Having a one line version makes it very hard to know if it's a reasonable input or not, and if the format changes, it would be really hard to edit to make correct (like if we decide we want the indent to be 3 spaces instead of 2).

+1

Modified.

This is a good example of why I don't think we should be mocking out the parsing. our tests shouldn't need to know exactly what value is passed to parse. If we later decide to change what we pass to include the name, for example, then this will break... but our test shouldn't actually care.

I think they should care, right? We're meant to be testing whether or not our command has correctly glued the user interaction together with the component which handles the parsing, right? It's the actual parsing that we don't care about; that is tested in the module that does the parsing.

Right now, if the expected input of Parse changes, you need to change two things in this package: the actual code that produces the yaml, AND this test.

If you didn't mock out Parse here, then when Parse changed, you'd only have to change the actual code that produces the yaml. This test would still test the interaction between the two, without having to encode knowledge in the test about what Parse expects.

I'm not sure if I understand this point. If we could change the expected input of ParseOneCloud, but not change any tests, isn't that a gap in testing?

It's not a gap in testing. A change to ParseOneCloud would cause this test to fail. The way to fix this test would be to fix the code for add-cloud so it produces the correct input for ParseOneCloud. Then the test would pass again, without having to update the test at all. That's the ideal. If you have to update the test every time something internal to the system changes, then the test knows too much about what it's testing. Sometimes it's unavoidable, but in this case, it's not.

The test should say "whatever we output from add-cloud should be parseable by ParseOneCloud". It doesn't actually care what we output so long as it parses into the right value. The test shouldn't care if ParseOneCloud takes yaml or json, it should care that add-cloud produces -something- that produces the correct output from ParseOneCloud.

I think I see the point you're trying to make. This comes back to functional vs. unit testing and what you're describing is a functional test. Please see this comment for why this is not logically correct. Summary: here, we only care about three things:

• Are we calling the correct function/method?
• Did we pass it what we expect?
• Do we handle the expected output correctly?

this is a neat idea, BTW. Definitely makes it easier to understand what these random values mean.

So, I find it confusing that we don't have a check here to check that the write value was written. I get that it is implicitly checked via the fake call to WritePersonalCloudMetadata.... but I don't like that it is so implicit. If we write out the wrong value, how will this test fail? It seems like it'll be a lot less clear that "expected foo, got bar".

If we write out the wrong value, the call will never be made and this function will return 0. You're correct that the arguments are tested implicitly. I think I disagree that this is a bad thing?

So, in this case, what the test is actually testing is that the old data is replaced by the new data in Write. But that is not at all clear from what this test is telling us. The only thing it is explicitly says it is checking is that write was called once. It doesn't have a line that says "check if what was written was correct". The problem is that the test is not obviously correct. You look at it and you think "hey, they're not even testing that what was written was correct". You actualy are, since the test will fail if the written value is incorrect, but that has been obscured. That makes it hard for people to understand what is being tested, and it makes it hard for people to come back and update the test later.

It also means that when the test fails, it'll give a really terrible failure message. Instead of a very obvious 'expected to write "foo: bar", instead wrote "baz: bat"', you'll get "expected 1, got 0". As a developer, it's going to be very unclear that the test is failing because it wrote the wrong thing, rather than thinking it just didn't write at all.

So, in this case, what the test is actually testing is that the old data is replaced by the new data in Write. [...] The only thing it is explicitly says it is checking is that write was called once. It doesn't have a line that says "check if what was written was correct".

I disagree with this premise. The test is correct in that it is only checking that it's gluing the bits together correctly, i.e.: correct function call, args, and results. This is what the tests is testing. See my prior comment about the role of unit/functional tests.

It also means that when the test fails, it'll give a really terrible failure message. Instead of a very obvious 'expected to write "foo: bar", instead wrote "baz: bat"', you'll get "expected 1, got 0".

We were both wrong; I forgot, that function call will never be made. If you don't put in the correct arguments, you'll get a panic[1] with the errant method call listed. I think maybe we can clean this up a little, but that's a PR to the mock code.

The check for whether or not the mocked function is called is to guard against multiple calls, or an unexpected call to another registered mock call.

[1]

PANIC: add_test.go:428: addSuite.TestInteractiveExistingNameNoOverride

[LOG] 0:00.000 DEBUG  Call: PublicCloudMetadata([[]])
[LOG] 0:00.000 DEBUG  Results: [map[] false <nil>]
[LOG] 0:00.000 DEBUG  Call: PersonalCloudMetadata([])
[LOG] 0:00.000 DEBUG  Results: [map[homestack:{openstack  [userpass access-key] http://homestack   [{london http://london/1.0  }] map[] map[]}] <nil>]
[LOG] 0:00.000 DEBUG  Call: ParseOneCloud([[101 110 100 112 111 105 110 116 58 32 49 57 50 46 49 54 56 46 49 46 54 10]])
[LOG] 0:00.000 DEBUG  Results: [{manual  [] 192.168.1.6   [] map[] map[]} <nil>]
[LOG] 0:00.000 DEBUG  Call: PersonalCloudMetadata([])
[LOG] 0:00.000 DEBUG  Results: [map[homestack:{openstack  [userpass access-key] http://homestack   [{london http://london/1.0  }] map[] map[]}] <nil>]
[LOG] 0:00.000 DEBUG  Call: WritePersonalCloudMetadata([map[homestack:{openstack  [userpass access-key] http://homestack   [{london http://london/1.0  }] map[] map[]} homestack2:{manual  [] 192.168.1.6   [] map[] map[]}]])
[LOG] 0:00.000 DEBUG  Results: []
... Panic: runtime error: index out of range (PC=0x45D80C)

/gnu/store/6i15c20ggyix8q4sy5z63izfwfkz24q9-go-1.7.3/src/runtime/panic.go:458
  in gopanic
/gnu/store/6i15c20ggyix8q4sy5z63izfwfkz24q9-go-1.7.3/src/runtime/panic.go:27
  in panicindex
add_test.go:56
  in fakeCloudMetadataStore.WritePersonalCloudMetadata
add.go:303
  in addCloud
add.go:165
  in addCloudCommand.runInteractive
add.go:112
  in addCloudCommand.Run
add_test.go:459
  in addSuite.TestInteractiveExistingNameNoOverride
/gnu/store/6i15c20ggyix8q4sy5z63izfwfkz24q9-go-1.7.3/src/reflect/value.go:302
  in Value.Call
/gnu/store/6i15c20ggyix8q4sy5z63izfwfkz24q9-go-1.7.3/src/runtime/asm_amd64.s:2086
  in goexit
OOPS: 15 passed, 1 PANICKED
--- FAIL: TestPackage (0.05s)
FAIL
FAIL	github.com/juju/juju/cmd/juju/cloud	0.095s

My point is that I can't tell that it's checking the args to Write by looking at the test, because there's no explicit code saying it's doing so. It's hidden by a couple layers of indirection. So, yes, it is testing the args, but only indirectly and implicitly, and that's bad, given that testing the args to Write is basically the entire purpose of this particular test. At the very least, we should add a comment to the place where we call Call() with the argument and note that this effectively tests that the right arguments are passed.

The fact that the test panics when it fails is pretty bad. That's a problem with the test code that needs to be fixed, IMO. If it's just a fix to the external code, that's fine, we can land that separately.